Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[automation] Added automation script StartLevels #2222

Merged
merged 1 commit into from
Mar 14, 2021

Conversation

jpg0
Copy link
Contributor

@jpg0 jpg0 commented Mar 3, 2021

Allows scripts to specify per-script start levels (which differ from the start level for the ScriptFileWatcher itself). Also extracted various functionality from ScriptFileWatcher into distinct components & added unit tests to cover all script loading functionality.

This fixes #2002 and possibly #1983 too. The vast majority of the new LOC are unit tests.

Signed-off-by: jpg0 [email protected]

@jpg0 jpg0 requested a review from a team as a code owner March 3, 2021 07:41
@jpg0 jpg0 force-pushed the script-start-levels branch from acd14d6 to 033eecd Compare March 3, 2021 07:42
@boc-tothefuture
Copy link

Allows scripts to specify per-script start levels.

Can you provide an example of how the script accomplishes this?

@jpg0
Copy link
Contributor Author

jpg0 commented Mar 3, 2021

I provided two mechanisms - either in the name of the script or the folder that contains it. So to run at start level 60:

.../script.sl60.py or
.../sl60/script.js

Happy to add documentation changes to the PR too, but I'm not sure where it goes.

@jpg0
Copy link
Contributor Author

jpg0 commented Mar 4, 2021

Found the docs at https://github.com/openhab/openhab-docs/blob/main/configuration/jsr223.md

Happy to create a PR for this too, just want to ensure that the maintainers are happy with this change before I do so.

(I also see that the current docs describe the loading order, which is no longer correct as of OH3.x)

@jpg0 jpg0 force-pushed the script-start-levels branch 2 times, most recently from 2deb01e to 03c3727 Compare March 6, 2021 04:31
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/use-start-level-reached-in-rule-file/118683/14

@jpg0 jpg0 force-pushed the script-start-levels branch 2 times, most recently from 467f333 to f9c2635 Compare March 11, 2021 03:37
slTrackers.remove(tracker);
}

public interface StartLevelTracker {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I deliberately didn't introduce a dedicated tracker here as it somewhat duplicates the already existing ReadyTracker - this service can be used to track start level changes, see this example.

Would you be ok to refactor your PR accordingly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaikreuzer sure! I've just pushed an update which switches to that patttern. Hope that is better.

(BTW I originally added the dedicated tracker to the StartLevelService because I didn't want to replicate the functionality that it provided - e.g. storing all the ready markers and determining the singular current start level. I've just used a much simpler implementation however.)

Allows scripts to specify per-script start levels which differ from the start level for the ScriptFileWatcher itself. Also extracted some functionality from ScriptFileWatcher into distinct components & added unit tests to cover all script loading functionality.

Signed-off-by: Jonathan Gilbert <[email protected]>
@jpg0 jpg0 force-pushed the script-start-levels branch from f9c2635 to a55a81d Compare March 14, 2021 03:26
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jpg0, lgtm!

And yes, it would be awesome if you could create a PR for the docs repo to cover this new feature there - thanks!

@kaikreuzer kaikreuzer merged commit 635b700 into openhab:main Mar 14, 2021
@kaikreuzer kaikreuzer added automation enhancement An enhancement or new feature of the Core labels Mar 14, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Mar 14, 2021
import java.util.regex.Pattern;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.jetbrains.annotations.NotNull;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use jetbrains null annotations.

See https://www.openhab.org/docs/developer/guidelines.html#null-annotations

Can you do a follow up PR with adding proper null annotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that! IDEA can be very keen on adding these sometimes; I hadn't noticed. I'll create a follow up PR to remove them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!


@NotNull
@Override
public <V> ScheduledFuture<V> schedule(@NotNull Callable<V> callable, long delay, @NotNull TimeUnit unit) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the jetbrains @NotNull annotations in this file should be replaced with Eclipse JDT annotaitons.

@kaikreuzer
Copy link
Member

Good catches, @wborn, sorry for having merged without noticing. 😲

@wborn
Copy link
Member

wborn commented Mar 14, 2021

Those jetbrains annotations seem to be a dependency of hivemq-mqtt-client. Maybe we can exclude it to prevent it from happening. They've ended up in some add-ons too (openhab/openhab-addons#10334).

@jpg0
Copy link
Contributor Author

jpg0 commented Mar 14, 2021

PR to remove the introduced annotations: #2241

@jpg0 jpg0 deleted the script-start-levels branch March 14, 2021 22:31
@jpg0
Copy link
Contributor Author

jpg0 commented Mar 14, 2021

@wborn Can we add a build step to reject imports from the org.jetbrains.annotations package? I just had a look to see if it would be easy in spotless (possibly, not sure as the annotations don't always have equivalents), but it could be done in the custom sat plugin stuff to just break the build.

@wborn
Copy link
Member

wborn commented Mar 14, 2021

SAT already scans for forbidden packages. But it will only result in warnings and not errors, so we could add the package to ruleset.properties

Excluding the dependency from hivemq-mqtt-client would also introduce another hurdle for using them. 😉

jpg0 added a commit to jpg0/openhab-docs that referenced this pull request Mar 15, 2021
JSR223 scripts can now be run at specific start levels as of openhab/openhab-core#2222

This PR adds the docs to describe how it can be used.
jpg0 added a commit to jpg0/openhab-docs that referenced this pull request Mar 15, 2021
JSR223 scripts can now be run at specific start levels as of openhab/openhab-core#2222

This PR adds the docs to describe how it can be used.

Signed-off-by: Jonathan Gilbert <[email protected]>
@jpg0
Copy link
Contributor Author

jpg0 commented Mar 15, 2021

PR for doc changes: openhab/openhab-docs#1511

Confectrician added a commit to openhab/openhab-docs that referenced this pull request Mar 15, 2021
* Startlevel execution support for jsr223 scripts

JSR223 scripts can now be run at specific start levels as of openhab/openhab-core#2222

This PR adds the docs to describe how it can be used.

Signed-off-by: Jonathan Gilbert <[email protected]>

* Add linebreaks.

Signed-off-by: Jerome Luckenbach <[email protected]>

Co-authored-by: Jerome Luckenbach <[email protected]>
@cweitkamp cweitkamp linked an issue Mar 16, 2021 that may be closed by this pull request
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/graalvm-for-automation/84116/21

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 11, 2023
Allows scripts to specify per-script start levels which differ from the start level for the ScriptFileWatcher itself. Also extracted some functionality from ScriptFileWatcher into distinct components & added unit tests to cover all script loading functionality.

Signed-off-by: Jonathan Gilbert <[email protected]>
GitOrigin-RevId: 635b700
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Script files not executed in order at startup Script files are loaded before scripting engine is available
5 participants